Skip to content

feat(cmake): centralize and harden compiler warning flags#357

Merged
bburda merged 10 commits intomainfrom
feature/production-warnings
Apr 6, 2026
Merged

feat(cmake): centralize and harden compiler warning flags#357
bburda merged 10 commits intomainfrom
feature/production-warnings

Conversation

@bburda
Copy link
Copy Markdown
Collaborator

@bburda bburda commented Apr 6, 2026

Pull Request

Summary

Centralize compiler warning configuration into a shared ROS2MedkitWarnings.cmake module, replacing duplicated per-package add_compile_options() blocks. Enable -Werror by default and add production-grade warning flags aligned with OpenSSF, AUTOSAR C++14, and MISRA C++ guidelines.

Key changes:

  • New ROS2MedkitWarnings.cmake module with 22+ warning flags (+ 4 GCC-only)
  • All 12 C++ packages now use include(ROS2MedkitWarnings) instead of per-package flags
  • -Werror enabled by default (configurable via ENABLE_WERROR cmake option)
  • ros2_medkit_relax_vendor_warnings() function to suppress strict warnings on gtest/gmock vendor targets
  • Vendored dynmsg excluded via set_source_files_properties
  • Auto-generated msgs package excluded from strict warnings
  • All warnings surfaced by new flags fixed in our code (~20 files)

New flags added: -Wswitch-enum, -Wfloat-equal, -Wcast-qual, -Wundef, -Wzero-as-null-pointer-constant, -Wextra-semi, -Wdouble-promotion, -Wsign-conversion, -Wnon-virtual-dtor, -Wold-style-cast, -Woverloaded-virtual, -Wcast-align, -Wnull-dereference, -Wimplicit-fallthrough, -Wformat=2, -Wuseless-cast (GCC), -Wduplicated-cond (GCC), -Wduplicated-branches (GCC), -Wlogical-op (GCC)

Types of code fixes: implicit type conversions (int64_t to int), useless casts, incomplete switch-enum coverage (missing enum values with default), float-to-double in variadic printf, char sign conversions, redundant std::string() wrappers.

Issue

Type

  • Bug fix
  • New feature or tests
  • Breaking change
  • Documentation only

Testing

  • Full colcon build --symlink-install passes with zero errors across all 14 packages
  • ./scripts/test.sh passes: 2446 tests, 0 errors, 0 failures, 0 skipped
  • Pre-commit hooks pass (clang-format, cmake-lint, ament-copyright, etc.)
  • Gtest/gmock vendor warnings suppressed correctly (warnings visible but not errors)

Checklist

  • Breaking changes are clearly described (and announced in docs / changelog if needed)
  • Tests were added or updated if needed
  • Docs were updated if behavior or public API changed

@bburda
feat(cmake): centralize compiler warnings with ROS2MedkitWarnings module
f691a6a 
Add ROS2MedkitWarnings.cmake with production-grade warning flags
aligned with OpenSSF, AUTOSAR C++14, and MISRA C++ guidelines.
Enable -Werror by default. Replace per-package add_compile_options
blocks with a single include(ROS2MedkitWarnings) across all 12
packages.

New flags: -Wswitch-enum, -Wfloat-equal, -Wcast-qual, -Wundef,
-Wzero-as-null-pointer-constant, -Wextra-semi, -Wdouble-promotion,
-Wsign-conversion, -Wnon-virtual-dtor, -Wold-style-cast,
-Woverloaded-virtual, -Wcast-align, -Wnull-dereference,
-Wimplicit-fallthrough, -Wformat=2, -Wuseless-cast (GCC),
-Wduplicated-cond, -Wduplicated-branches, -Wlogical-op (GCC).

Fix all warnings surfaced by the new flags: implicit conversions,
useless casts, incomplete switch-enum coverage, float-to-double in
printf, char sign conversions.

Exclude vendored dynmsg and gtest/gmock from strict warnings via
set_source_files_properties and ros2_medkit_relax_vendor_warnings().

Closes #356
Copilot AI review requested due to automatic review settings April 6, 2026 14:22
@bburda bburda self-assigned this Apr 6, 2026
@bburda bburda requested a review from mfaferek93 April 6, 2026 14:22
Copilot AI reviewed Apr 6, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR centralizes compiler warning configuration for the ros2_medkit C++ workspace into a shared CMake module, enabling stricter warnings (and -Werror by default) and updating code to satisfy the expanded warning set.

Changes:

  • Added ROS2MedkitWarnings.cmake (strict warnings + optional -Werror) and replaced per-package add_compile_options() blocks with include(ROS2MedkitWarnings).
  • Added ros2_medkit_relax_vendor_warnings() to prevent vendored gtest/gmock warnings from failing builds under -Werror, and relaxed warnings for selected vendored dynmsg sources.
  • Updated multiple C++ sources to address newly-enabled warnings (e.g., switch-enum coverage, format/printf type correctness, signed/unsigned conversions).

Reviewed changes

Copilot reviewed 45 out of 45 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/ros2_medkit_serialization/src/json_serializer.cpp Completes YAML NodeType switch coverage for stricter -Wswitch-enum.
src/ros2_medkit_serialization/CMakeLists.txt Switches to centralized warnings module; relaxes warnings for vendored dynmsg sources; relaxes vendor test warnings.
src/ros2_medkit_plugins/ros2_medkit_graph_provider/CMakeLists.txt Replaces local warning flags with centralized module; relaxes vendor test warnings.
src/ros2_medkit_msgs/CMakeLists.txt Removes per-package compile options (consistent with “exclude generated msgs from strict warnings” intent).
src/ros2_medkit_integration_tests/demo_nodes/long_calibration_action.cpp Fixes signed/unsigned indexing warnings in Fibonacci sequence logic.
src/ros2_medkit_integration_tests/demo_nodes/brake_actuator.cpp Fixes printf-type correctness for float passed to varargs logging macro.
src/ros2_medkit_integration_tests/CMakeLists.txt Replaces local warning flags with centralized module.
src/ros2_medkit_gateway/test/test_resource_sampler_registry.cpp Removes redundant std::string(...) wrapper (warning cleanup).
src/ros2_medkit_gateway/src/updates/update_manager.cpp Expands switch handling to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/openapi/capability_generator.cpp Expands enum switch handling to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/native_topic_sampler.cpp Handles additional enum values to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/models/thread_safe_entity_cache.cpp Adds missing enum cases to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/models/aggregation_service.cpp Adds missing enum cases to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/log_manager.cpp Adjusts integer conversion handling for stricter warnings.
src/ros2_medkit_gateway/src/lock_manager.cpp Adds missing enum cases to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/http/handlers/update_handlers.cpp Expands switch handling to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/http/handlers/trigger_handlers.cpp Expands switch handling to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/http/handlers/sse_fault_handler.cpp Adjusts logging argument types for stricter format warnings (but currently introduces a portability risk).
src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp Adds missing enum cases to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp Adds missing enum cases to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp Fixes signed/unsigned conversion warning for std::count result.
src/ros2_medkit_gateway/src/http/handlers/cyclic_subscription_handlers.cpp Expands enum switch handling to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp Expands enum switch handling to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp Fixes integer literal type for JSON value extraction to avoid conversion warnings.
src/ros2_medkit_gateway/src/http/entity_path_utils.cpp Expands enum switch handling to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/gateway_node.cpp Expands switch handling and removes redundant std::string(...) wrappers.
src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp Expands enum switch handling to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/discovery/manifest/manifest_parser.cpp Adds explicit enum case to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/discovery/discovery_manager.cpp Adds explicit enum case to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/discovery/discovery_enums.cpp Adds explicit enum case to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/configuration_manager.cpp Adds explicit enum cases to satisfy -Wswitch-enum.
src/ros2_medkit_gateway/src/aggregation/peer_client.cpp Fixes std::isalnum usage by ensuring unsigned-char promotion.
src/ros2_medkit_gateway/CMakeLists.txt Replaces local warning flags with centralized module; relaxes vendor test warnings.
src/ros2_medkit_fault_reporter/CMakeLists.txt Replaces local warning flags with centralized module; relaxes vendor test warnings.
src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp Removes potentially warning-triggering redundant cast.
src/ros2_medkit_fault_manager/src/fault_manager_node.cpp Adjusts parameter typing/logging (but currently introduces a format-spec mismatch).
src/ros2_medkit_fault_manager/CMakeLists.txt Replaces local warning flags with centralized module; relaxes vendor test warnings.
src/ros2_medkit_discovery_plugins/ros2_medkit_topic_beacon/CMakeLists.txt Replaces local warning flags with centralized module; relaxes vendor test warnings.
src/ros2_medkit_discovery_plugins/ros2_medkit_param_beacon/CMakeLists.txt Replaces local warning flags with centralized module; relaxes vendor test warnings.
src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/CMakeLists.txt Replaces local warning flags with centralized module; relaxes vendor test warnings.
src/ros2_medkit_discovery_plugins/ros2_medkit_beacon_common/CMakeLists.txt Replaces local warning flags with centralized module; relaxes vendor test warnings.
src/ros2_medkit_diagnostic_bridge/CMakeLists.txt Replaces local warning flags with centralized module; relaxes vendor test warnings.
src/ros2_medkit_cmake/CMakeLists.txt Installs the new warnings module.
src/ros2_medkit_cmake/cmake/ROS2MedkitWarnings.cmake New centralized warning policy (+ optional -Werror) and vendor test relaxation helper.
src/ros2_medkit_cmake/cmake/ros2_medkit_cmake-extras.cmake Updates module-path documentation comment to mention ROS2MedkitWarnings.

bburda added 6 commits April 6, 2026 16:55
@bburda
fix(cmake): remove -Wnull-dereference due to GCC 13 STL false positives
5e15467 
GCC 13 with optimization triggers false -Wnull-dereference warnings
on standard library headers (streambuf inlining), breaking CI builds.
This is a known GCC issue. Null dereference checks are covered by
clang-tidy instead.
@bburda
fix(cmake): restore -Wnull-dereference, default ENABLE_WERROR to OFF
451b6b2 
Restore -Wnull-dereference which was removed due to GCC 13 false
positives on STL headers. With ENABLE_WERROR defaulting to OFF, all
warning flags remain active for visibility without breaking the build
on different GCC versions. ENABLE_WERROR can be toggled ON once
external includes are treated as system headers.
@bburda
feat(cmake): use selective -Werror for flags safe from external headers
4028a53 
Replace blanket ENABLE_WERROR=OFF with selective -Werror=<flag> for
warnings that only fire on our code: shadow, switch-enum, old-style-cast,
float-equal, cast-qual, undef, zero-as-null-pointer-constant, extra-semi,
overloaded-virtual, non-virtual-dtor, implicit-fallthrough, format,
duplicated-cond/branches, logical-op, useless-cast (GCC).

Flags that false-positive on external headers (STL, ROS 2, nlohmann,
gtest) remain as warnings only: conversion, sign-conversion,
double-promotion, null-dereference, cast-align.

The relax function now suppresses specific promoted flags on gtest/gmock
targets instead of blanket -Wno-error.
@bburda
fix(cmake): suppress all warnings on vendored and gtest code with -w
2d75f67 
Selective -Werror=<flag> overrides blanket -Wno-error, so per-flag
suppression lists are fragile and break across GCC versions. Use -w
(suppress all warnings) on vendored dynmsg sources and gtest/gmock
targets instead - these are third-party code we don't modify.
@bburda
fix(cmake): demote -Wuseless-cast from error to warning
a6ec484 
Type aliases (time_t, int64_t, size_t) resolve to different underlying
types across distros. A static_cast needed for portability on Jazzy
(GCC 13) becomes useless on Humble (GCC 11) where the types match.
Keep the warning for visibility but don't fail the build.
@bburda
fix(cmake): mark vendored cpp-httplib include as SYSTEM
eadf4ae 
The vendored httplib.h header triggers strict warnings (switch-enum,
old-style-cast, format-nonliteral) which become errors with our
selective -Werror flags. Mark the vendored include directory as SYSTEM
so compiler warnings from httplib.h are suppressed. On Jazzy the
system package is used via pkg-config which already treats includes
as system headers.
@bburda bburda requested a review from Copilot April 6, 2026 16:35
Copilot AI reviewed Apr 6, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated 1 comment.

@bburda bburda mentioned this pull request Apr 6, 2026
Open
@bburda
fix(gateway,fault_manager): fix format specifier portability issues
385fe13 
Use PRId64 with static_cast<int64_t> for portable printf formatting
of parameter values and chrono durations. The underlying type of
declare_parameter<int> and chrono::milliseconds::rep varies across
platforms (long vs long long), so explicit casts ensure format
specifier agreement.
mfaferek93
mfaferek93 reviewed Apr 6, 2026
View reviewed changes
@bburda
fix(cmake): namespace ENABLE_WERROR option as MEDKIT_ENABLE_WERROR
a006a96 
Avoid potential collision with other packages in colcon workspace
when passing cmake args via colcon build --cmake-args.
mfaferek93
mfaferek93 reviewed Apr 6, 2026
View reviewed changes
@bburda
fix: address review feedback on warning infrastructure
a2dc0ec 
- Separate ParameterErrorCode::NONE from error cases in
  classify_error_code with comment explaining it is unreachable
- Add comment to ros2_medkit_msgs explaining why ROS2MedkitWarnings
  is not included (rosidl-generated code)
- Simplify format specifier in fault_manager_node: use %ld directly
  instead of PRId64 + static_cast (declare_parameter<int> returns long)
@mfaferek93 mfaferek93 requested review from Copilot and mfaferek93 April 6, 2026 19:39
Copilot AI reviewed Apr 6, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 45 out of 45 changed files in this pull request and generated 1 comment.

@bburda bburda merged commit 22d9b55 into main Apr 6, 2026
18 checks passed
@bburda bburda deleted the feature/production-warnings branch April 6, 2026 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@mfaferek93 mfaferek93 mfaferek93 approved these changes

Assignees

@bburda bburda

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Centralize and harden compiler warning flags

3 participants

@bburda @mfaferek93